-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Fix duplicate pod creation in KubernetesJobOperator #53368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicate pod creation in KubernetesJobOperator #53368
Conversation
0e04769 to
a9ad704
Compare
d6d3fa4 to
8b43bca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM - got some minor comments.
Also, I'll be happy for an additional review :)
providers/cncf/kubernetes/tests/unit/cncf/kubernetes/operators/test_job.py
Outdated
Show resolved
Hide resolved
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/operators/job.py
Outdated
Show resolved
Hide resolved
30445de to
f123b1a
Compare
|
In reference to #49899 - I think a change is still necessary to avoid creating multiple pods when parallelism is not set. Do we want to change the control flow to use |
You could start with handling only the case where |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
d561015 to
b0f5289
Compare
|
LGTM, I'm making sure with the other contributors that handling If no strong objections are given in the next 1-2 days, or there are additional approvals for this PR by then - I'm ok with merging it as-is. |
|
@stephen-bracken / @shahar1 as concerns raised in https://apache-airflow.slack.com/archives/C06K9Q5G2UA/p1767429130216899 - one thing to consider maybe is - I'd propose to - adding a newsfragment such that it is highlighted in the changelog of the provider. TLDR: Requesting to add a newsfragment to highlight this interface change. UPDATE: Args, providers have no newsfragmen, add it to changelogs like in https://github.com/apache/airflow/pull/59143/changes#diff-24cff4e7b7926b95f4efef45da9f9d6f43b237b5143990b1554113251cb2c12eR30 for example that it is included in next providers wave. |
8befa0f to
5a29826
Compare
@jscheffl Thanks, added a changelog note. |
5a29826 to
50261b1
Compare
50261b1 to
6b3dc30
Compare
|
Drafting the PR following the author's request to make some changes, please do not merge |
shahar1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
faae3ae to
d1265cd
Compare
d1265cd to
522f30f
Compare
522f30f to
28333ae
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
|
Great job @stephen-bracken ! |
Fix KubernetesJobOperator.get_or_create_pod() sometimes creating duplicate pods.
(re-raised from #52885)
during
execute()the KubernetesJobOperator attempts to find the pod associated with the Job object usingself.get_or_create_pod(). If Kubernetes is being slow then the Job object will not create a pod before this method gets called, which will result in the underlyingfind_pod()method returningNone, and a duplicate headless pod being created for this task.This PR removes references to the
get_or_create_pod()method in favour ofKubernetesJobOperator.get_pod()to prevent creating headless pods.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.